[th/node-name-cleanup] cleanups around node name#219
Merged
Conversation
thom311
commented
May 23, 2025
Contributor
- ktoolbox: bump to latest version
- task: simplify selection of pod template and rename pod files
- task: add "Task.render_pod_file()" helper
- task: avoid redundant Task.out_file_yaml state
- tftbase: add str_sanitize() function for sanitizing string
- task: sanitize unusual characters in the pod name
- task: make "node_name_sanitized" a property
- testConfig: validate ConfNodeBase.name to be a valid DNS name
- manifests: quote the {node,pod}_name Jinja variables in YAML
08f5897 to
3b3073d
Compare
Contributor
|
The regex parsing doesn't work for my node name |
033beec to
00dd1d7
Compare
m-naught
reviewed
May 30, 2025
Signed-off-by: Thomas Haller <thaller@redhat.com>
The "if" checks had duplicated code. For example, they all called tftbase.get_manifest(), which can be done at the end of the block. Also, we already chose a unique and suitable name for the pod. For the generated YAML file, we should just use the same name. Previously, the generated name would often differ from the pod name for no reason. Signed-off-by: Thomas Haller <thaller@redhat.com>
There are two cases for calling Task.render_file(): - we render the file of the pod itself, that is, using self.in_file_template and self.out_file_yaml. - we render other resources, where the caller specifies the in and out file parameters. In no case, does the caller only want to specify either the in_file_template or the out_file_yaml parameter. This is confusing. Add Task.render_pod_file(). Also, change the log string for plugin pods from "Server Pod Yaml" to "Plugin Pod Yaml". Signed-off-by: Thomas Haller <thaller@redhat.com>
The property is always directly dependent on the pod_name. Don't track it separately. Calculate it when needed. Signed-off-by: Thomas Haller <thaller@redhat.com>
Preserves ASCII lower case letters and digits. But other characters are escaped using "-". The goal is that the input unambiguously maps to a result string (which is reversible in theory). Also, leading and trailing '-' are escaped too, because those are not valid in a DNS label. We will use this for the pod-name. Signed-off-by: Thomas Haller <thaller@redhat.com>
Use tftbase.str_sanitize() for this. Signed-off-by: Thomas Haller <thaller@redhat.com>
This seems more pythonic. The getter has no side effects and is cheap to evaluate. Also, it is based on the "node_name". That property is based on immutable attributes and thus yields the identical reference on each call. This makes "node_name" stateless and simple to reason about. As such, "node_name_sanitized" also gives always an equal value, albeit a newly allocated string reference. Signed-off-by: Thomas Haller <thaller@redhat.com>
We use the node name to derive the pod_name. For that usage we have
"node_name_sanitize", which handles any characters.
Otherwise, we also use the node name for the pod selector:
nodeSelector:
kubernetes.io/hostname: {{ node_name }}
This hostname is usually the same as the node name. The node name must
be a valid DNS name. Technically, it seems you could configure the
hostname to differ from the node name, and then the hostname is not
required to be a valid DNS name. But let's not support that.
So this change breaks support for systems that rename the hostname of a
node to no longer be a valid DNS name. Just don't do that. Not
supported.
What this gives is that we know that the node name does not contain
bogus characters. For example, our naive Jinja2 replacement in the pod
files is not aware that it needs to insert a YAML string. Now we at
least know that the node name won't contain characters like '"' or '['
that would break the YAML.
Signed-off-by: Thomas Haller <thaller@redhat.com>
Jinja is not aware of what it is templating. So it does not know that we want to set a string in YAML, or how to quote that. Luckily, we already ensure that the node name is a valid DNS name. The form of the pod name is even more strict. So they cannot contain quotes that could break our YAML. Still, a node name "1" would be interpreted as a plain number in YAML. We always want a YAML string. Quote the value in the template. In this case, this is safe, because we know that the node name and the pod name only contain certain characters. Signed-off-by: Thomas Haller <thaller@redhat.com>
033beec to
d9e112b
Compare
Contributor
|
LGTM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.